chore: integrate storageService with tokenListController #7413
Conversation
| tokensChainsCache: { | ||
| includeInStateLogs: false, | ||
| persist: true, | ||
| persist: false, // Persisted separately via StorageService |
There was a problem hiding this comment.
Making this false to block disk writes
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…oading and saving
…tchTokenList calls
|
|
||
| // Load cache from StorageService on initialization and handle migration. | ||
| // Store the promise so other methods can await it to avoid race conditions. | ||
| this.#initializationPromise = this.#initializeFromStorage(); |
There was a problem hiding this comment.
Generally it's discouraged to make async calls in the constructor. This can complicate unit tests (both here and in modules that import this), and it gives less control to the caller over how initialization happens.
For example, we're been looking more lately at speeding up initialization, potentially delaying slower initialization steps until more critical steps have finished already (especially on mobile). If this happens in the constructor, there is no way to delay it without delaying construction of the controller itself (which would be quite complex).
Instead what we recommend is to move this to an init function, and update both clients to call that init function after construction. A number of other controllers already, e.g. AccountTreeController. This is easier to test, easier to mock on the platform side, and makes it easy for the platform team to optimize initialization.
| * | ||
| * @returns A promise that resolves when migration is complete. | ||
| */ | ||
| async #migrateStateToStorage(): Promise<void> { |
There was a problem hiding this comment.
We could do this in a client migration instead. That's where we usually migrate state.
| * - Successfully removed chains are cleared from state | ||
| * - Failed removals are kept in state to maintain consistency with storage | ||
| * | ||
| * Acquires the mutex to prevent race conditions with fetchTokenList. |
There was a problem hiding this comment.
Why do we need a mutex here? I don't understand what race condition we're protecting against here.
There was a problem hiding this comment.
I would recommend that we keep the persisted state synchronized at all times, rather than updating it in each location where state is updated. That ensures we won't forget to update it somewhere, and avoids duplication, and makes it easier to reason about overlapping operations/race conditions.
e.g. we could do something like this:
this.messenger.subscribe(
'TokenListController:stateChange',
(tokensChainsCache: TokenListControllerState['tokensChainsCache']) => persistCache(tokensChainsCache),
(state: TokenListControllerState) => state.tokensChainsCache,
Then within persistCache, we can do things like:
- protect against concurrent operations
- debounce state updates so they aren't saved too frequently
All without using a mutex. Better to avoid the need for a mutex if possible, since waiting for a mutex can take an unpredictable amount of time, and it's easy to accidentally create deadlocks.
Description
Optimizes TokenListController storage to reduce write amplification by persisting tokensChainsCache via StorageService using per-chain files instead of a single monolithic state property.
Related: https://github.com/MetaMask/metamask-mobile/pull/22943/files
Related: https://github.com/MetaMask/decisions/pull/110
Related: #7192
Explanation
The tokensChainsCache (~5MB total, containing token lists for all chains) was persisted as part of the controller state. Every time a single chain's token list was updated (~100-500KB), the entire ~5MB cache was rewritten to disk, causing:
Solution
Per-Chain File Storage:
Each chain's cache is now stored in a separate file (e.g., tokensChainsCache:0x1, tokensChainsCache:0x89)
Only the updated chain (~100-500KB) is written on each token fetch, reducing write operations by ~90-95%
All chains are loaded in parallel at startup to maintain compatibility with TokenDetectionController
Key Changes:
References
Checklist
Note
BREAKING:
TokenListControllerpersiststokensChainsCacheviaStorageServiceusing per-chain files; state metadata fortokensChainsCacheset topersist: false.#initializeFromStorage,#loadCacheFromStorage(parallel per-chain load),#saveChainCacheToStorage, and#migrateStateToStoragefetchTokenListto await init, write only the updated chain, and avoid refreshing timestamps on failed fetchesclearingTokenListDataasync to remove all per-chain files (with partial-failure handling) and clears state accordingly#fields, polling stop method) and improves network-change handling@metamask/storage-servicedependency and tsconfig references; updates changelogWritten by Cursor Bugbot for commit 71124a6. This will update automatically on new commits. Configure here.